Mitigate exception thrown on returningBroken connection to the pool#4519
Closed
ggivo wants to merge 1 commit into
Closed
Mitigate exception thrown on returningBroken connection to the pool#4519ggivo wants to merge 1 commit into
ggivo wants to merge 1 commit into
Conversation
A behavioral regression was introduced in commons-pool 2.13.0 as part of the fix for POOL-424, which ensures that invalidateObject() replaces the destroyed instance by internally calling addObject(). This leads to exception being propagated in case new Jedis/Connection can not be established. This commit introduce a mitigation code to allow Jedis to wotk with 2.13.1 till apache/commons-pool#454 is resolved
🛡️ Jit Security Scan Results
✅ No security findings were detected in this PR
Security scan by Jit
|
Collaborator
Author
|
Discussed with the team and decided it introduces risks of future incompatibility with the library. Closing in favour of |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Mitigate POOL-431 regression:
returnBrokenResourcethrows when upstream is downA behavioral regression was introduced in commons-pool 2.13.0 as part of the fix for POOL-424:
GenericObjectPool#invalidateObjectnow unconditionally replaces the destroyed instance by internally callingaddObject(). When the upstream Redis is unreachable, the implicit replacement fails and the exception propagates out ofJedis#close()/pool.returnBrokenResource(...), breaking the pre-2.13 contract where returning a broken connection was a silent no-op on the failure path. This is tracked upstream as POOL-431.Related PR's:
MultiDbClient.This change moves the mitigation down to
Pool<T>so every Jedis pool benefits. A thread-local guard set acrossreturnBrokenResource(...)lets anaddObject()override swallow failures originating from the implicit auto-replaceNote
Medium Risk
Touches core connection pooling behavior by changing how broken resources are invalidated under commons-pool2 2.13.x; mistakes could hide real pool/factory failures or affect pool liveness under load.
Overview
Restores pre-commons-pool2-2.13 behavior where closing/returning a broken Jedis resource does not throw if Redis is down, by adding a thread-local guard in
Pool.returnBrokenResourceand overridingaddObject()to suppress only the implicit auto-replacement failure triggered byinvalidateObject().Upgrades
commons-pool2to2.13.1and adds coverage: a newPoolUnitTestfor suppression/propagation semantics, plus an updatedUnavailableConnectionTestthat uses Toxiproxy (newredis-unavailableendpoint + docker-compose port) to reliably simulate an unreachable Redis instance.Reviewed by Cursor Bugbot for commit 62d001f. Bugbot is set up for automated code reviews on this repo. Configure here.